Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid stack overflow when many lookup redirection happens #25

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

BewareMyPower
Copy link
Owner

TODO: dig into the reason

### Motivation

When the host of the redirect URL cannot be resolved, segmentation fault
will happen:
https://github.com/apache/pulsar-client-cpp/blob/0bbc15502905d19c630d237b5e102bfb996bb098/lib/CurlWrapper.h#L173-L175

In this case, `curl` will be `nullptr`. Assigning a nullptr to a
`std::string` is an undefined behavior that might cause segfault.

### Modifications

Check if `url` is nullptr in `CurlWrapper::get` and before assigning it
to the `redirectUrl` field. Improve the
`HTTPLookupService::sendHTTPRequest` method by configuring the
`maxLookupRedirects` instead of a loop and print more detailed error
messages.

### Verifications

It's hard to mock a redirect case so I have to modify the
`TopicLookupBase#internalLookupTopicAsync` in broker side and return a
307 response.

When I return a `http://unknown:8080` as the redirect URL:

```
2023-11-23 17:06:12.821 ERROR [0x16d31b000] HTTPLookupService:246 | Response failed for url http://localhost:8080/lookup/v2/topic/persistent/public/default/my-topic: Couldn't resolve host name, curl error: Could not resolve host: unknown
```

Before this patch, it will crash.

When I just return a `http://localhost:8080` as the redirect URL:

```
2023-11-23 17:15:06.267 ERROR [0x16d657000] HTTPLookupService:243 | Response received for url http://localhost:8080/lookup/v2/topic/persistent/public/default/my-topic: Number of redirects hit maximum amount, curl error: Maximum (20) redirects followed, redirect URL: http://localhost:8080/lookup/v2/topic/persistent/public/default/my-topic?authoritative=false
```

Before this patch, it will print similar logs 20 times.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant